fix(compostition): Fix port of `@composeDirective` by sachindshinde · Pull Request #9164 · apollographql/router

sachindshinde · 2026-04-09T20:06:13Z

While reviewing #8936, I noticed some issues in the PR, but there were also more issues in the surrounding code for @composeDirective. This PR brings the Rust port more inline with the JS code (including the changes that #8936 were trying to port over). There were also a fair amount of issues with the tests, and I've similarly fixed those.

apollo-librarian · 2026-04-09T20:06:17Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 934c34d39b08f82bcc092113
Build Logs: View logs

✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

github-actions · 2026-04-09T20:07:27Z

@sachindshinde, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

dariuszkuc

Looks good just a couple questions around tests

dariuszkuc · 2026-04-10T18:44:05Z

        );

        let result = compose(vec![subgraph_a, subgraph_b]).unwrap();
+        println!("{:?}", result.hints());


leftover from debugging?

dariuszkuc · 2026-04-10T19:02:15Z

    pub spec_alias: Option<Name>,
    pub imports: Vec<Arc<Import>>,
    pub purpose: Option<Purpose>,
+    pub line_column_range: Option<Range<LineColumn>>,


note for self: this is added so we can calculate location information for subgraph errors

dariuszkuc · 2026-04-10T19:06:56Z

            let imports = directives
                .iter()
-                .map(|(original, alias)| {
+                .map(|(alias, original)| {


good catch!

dariuszkuc · 2026-04-10T19:09:02Z

@@ -1687,18 +1684,14 @@ format!("Field \"{field}\" of {} type \"{}\" is defined in some but not all subg
        // We should skip the supergraph specific directives, that is the @core and @join directives.


can you update this comment to @link and @join directives?

dariuszkuc · 2026-04-10T19:14:09Z

+        );
+        let subgraph_b = generate_subgraph(
+            "subgraphB",
+            r#"@link(url: "https://specs.custom.dev/foo/v1.1", import: ["@bar"])"#,


just wondering whether we should be explicit with import: ["@bar", "@foo"] here (or maybe use different custom directive names as @foo is implicitly handled as its name matches the spec name)

dariuszkuc · 2026-04-10T19:15:32Z

+            r#"@composeDirective(name: "@foo")"#,
+            r#"
+            directive @foo(name: String!) on FIELD_DEFINITION
+            directive @bar(name: String!) on FIELD_DEFINITION


should this only include @foo definition and skip the @bar (and vice versa in subgraph_b)?

dariuszkuc · 2026-04-10T19:16:47Z

+            r#"@composeDirective(name: "@foo")"#,
+            r#"
+            directive @foo(name: String!) on FIELD_DEFINITION
+            directive @bar(name: String!) on FIELD_DEFINITION


drop the @bar

… most likely)

sachindshinde · 2026-04-13T21:00:18Z

@dariuszkuc
Finished auditing the tests (both the existing ones, and the new ones). There were a fair amount of issues there too, I ended up comparing side-by-side with the JS and what's been pushed up should be matching now (there's one or two cases where I think in the Rust tests we decided some JS tests subsumed each other/were too similar so we didn't port them; from what I could see those cases were fine to not port).

dariuszkuc · 2026-04-13T21:35:50Z

-            directive_text,
+            r#"
+            directive @foo(name: String!) on FIELD_DEFINITION
+            directive @bar(name: String!, address: String) on FIELD_DEFINITION | OBJECT


do we need the bar in 1.0 subgraph?

Nah, it's just copied over from the JS tests (it's named "exported directive not imported everywhere. named consistently" in JS tests). The definition shouldn't be affecting/testing anything.

sachindshinde requested a review from a team as a code owner April 9, 2026 20:06

sachindshinde force-pushed the sachin/fix-compose-directive branch from 5a5c008 to be72cb3 Compare April 9, 2026 20:07

sachindshinde force-pushed the sachin/fix-compose-directive branch from 3a8755d to a532747 Compare April 10, 2026 01:28

sachindshinde mentioned this pull request Apr 10, 2026

fix(composition): relax compose directive validations when custom directives are absent in some graphs #8936

Closed

10 tasks

sachindshinde force-pushed the sachin/fix-compose-directive branch 3 times, most recently from 5dcf94a to b809f3d Compare April 10, 2026 17:21

dariuszkuc reviewed Apr 10, 2026

View reviewed changes

dariuszkuc mentioned this pull request Apr 13, 2026

fix(composition): fix cache tag supergaph spec and join__directive logic #9185

Merged

sachindshinde added 3 commits April 13, 2026 13:38

Fix @composeDirective handling

c33c964

Bump dhat limits (increased due to storing line_column_range in Link,…

ff7f177

… most likely)

Fix @composeDirective tests

ac63648

sachindshinde force-pushed the sachin/fix-compose-directive branch from b809f3d to ac63648 Compare April 13, 2026 20:38

Fix comment in merge_directive_definitions()

ea5f114

sachindshinde requested a review from dariuszkuc April 13, 2026 21:00

dariuszkuc reviewed Apr 13, 2026

View reviewed changes

dariuszkuc approved these changes Apr 13, 2026

View reviewed changes

sachindshinde force-pushed the sachin/fix-compose-directive branch from 09f7e0f to ea5f114 Compare April 13, 2026 21:50

sachindshinde enabled auto-merge (squash) April 13, 2026 21:51

sachindshinde merged commit 310d7b5 into dev Apr 13, 2026
18 checks passed

sachindshinde deleted the sachin/fix-compose-directive branch April 13, 2026 22:07

		@@ -1687,18 +1684,14 @@ format!("Field \"{field}\" of {} type \"{}\" is defined in some but not all subg
		// We should skip the supergraph specific directives, that is the @core and @join directives.

Conversation

sachindshinde commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apollo-librarian bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview has no changes

✅ AI Style Review — No Changes Detected

Uh oh!

github-actions bot commented Apr 9, 2026

Uh oh!

dariuszkuc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sachindshinde commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachindshinde Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sachindshinde commented Apr 9, 2026 •

edited

Loading

apollo-librarian bot commented Apr 9, 2026 •

edited

Loading

sachindshinde Apr 13, 2026 •

edited

Loading